-
-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wiring up new homepage to new dashboard #281
Conversation
change account to base levelunlock/unlock-app/src/components/creator/Dashboard.js Lines 26 to 31 in b96c624
This comment was generated by todo based on a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the logic here should be handled by react-router.
render() { | ||
if (!this.props.account) { | ||
return null //loading | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably eventually handle that at a higher level.. But that'll do for now!
} | ||
|
||
const mapDispatchToProps = dispatch => ({ | ||
setTransaction: (transaction) => dispatch(setTransaction(transaction)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we actually calling this anywhere in the component?
|
||
dashboardClick() { | ||
this.props.history.push('/dashboard') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, I would expect react router to handle that for you?
@@ -13,7 +23,7 @@ export default class Home extends PureComponent { | |||
Unlock is a protocol which enables creators to monetize their content with a few lines of code in a fully decentralized way. | |||
</Headline> | |||
|
|||
<Action> | |||
<Action onClick={this.dashboardClick}> | |||
<HomepageButton>Go to Your Dashboard</HomepageButton> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, you should probably use Link
from react-router-dom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked things and I really think you should use react router's stuff for this. LMK if you need help with it though!
Yep, I'm working on the changes! |
Using router. PTAL |
<HomepageButton>Go to Your Dashboard</HomepageButton> | ||
<ButtonLabel>Requires a browser with an Ethereum wallet</ButtonLabel> | ||
</Action> | ||
<Link to={'/dashboard'}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll know what I say about nesting components!... but I guess you tried without?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did, yeah. I have some feature requests for the next version of styled-components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol. happy to hear them!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 small nits!
@@ -51,6 +55,10 @@ export default class Home extends PureComponent { | |||
} | |||
} | |||
|
|||
Home.propTypes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably get rid of this ;)
No description provided.